Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge ostree-rs into this repository #2575

Merged
merged 434 commits into from
May 9, 2022

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Mar 31, 2022

Starting the ball rolling on #2427

Implemented by following https://stackoverflow.com/questions/1425892/how-do-you-merge-two-git-repositories - specifically git filter-repo --to-subdirectory-filter rust. So if you have a different opinion on what the subdirectory should be named (rust/bindings? in prep for merging ostree-rs-ext too?) now's the time to say!

I pushed one small commit on top which fixes up the build system, but that's it so far.

@jmarrero
Copy link
Member

jmarrero commented Apr 1, 2022

Would ostree-rs-ext live under the same rust dir? If so rust/lib/src? and rust/bindings/src? or both merged under rust/src?

@cgwalters
Copy link
Member Author

Would ostree-rs-ext live under the same rust dir? If so rust/lib/src? and rust/bindings/src? or both merged under rust/src?

Yeah, we should figure this out now. Hmm...how about rust-bindings/ for ostree-rs and rust-ext for ostree-ext?

(The thing is this topic really opens the question of the C library hard depending on Rust code, and that adds another wrinkle to this) I guess that could just be rust?

@jmarrero
Copy link
Member

jmarrero commented Apr 1, 2022

Yeah, we should figure this out now. Hmm...how about rust-bindings/ for ostree-rs and rust-ext for ostree-ext?

I like that.

(The thing is this topic really opens the question of the C library hard depending on Rust code, and that adds another wrinkle to this) I guess that could just be rust?

Do you mean like the C code would depend on the rust folder but inside that folder we will have rust-bindings and ostree-ext?
If so... I think I that makes sense to me.

rust/rust-bindings & rust/ostree-ext

@jlebon
Copy link
Member

jlebon commented Apr 4, 2022

No strong opinions on where the bindings live. Having it be rust-bindings/ SGTM.

It has become less clear to me however whether ostree-rs-ext should live in this repo. It'd be good I think to hold off on that until we sort out if/when we could have libostree hard-require Rust.

@cgwalters
Copy link
Member Author

OK a few things here. First a rhel8 branch exists that...well, we could rename it 2022-branch or something. ("stable" is IMO something that becomes meaningless over time, what happens with two stable branches etc.)

It'd be good I think to hold off on that until we sort out if/when we could have libostree hard-require Rust.

Yeah. I think I'd like to say that we (the upstream/git main) can, and anyone who can't follow can use the rhel8/2022-branch for now. We (upstream) will still maintain the 2022 branch for stability and security fixes.

It's not like we're going to go on a wholesale rewrite anytime soon, so even if we make Rust a hard requirement it's very likely that most things will be easily backportable.

cgwalters and others added 13 commits May 6, 2022 12:53
I'm trying to debug a problem in ostree-rs-ext, and it's
handy to be able to do `dbg!(mtree.copy_files())`.
The underlying `ostree_repo_load_file()` API has the caller pass
`NULL` for output arguments it doesn't want.  This isn't sanely
bindable in Rust - what the generator does is always request
all values, but maps them all to `Option<T>`.

The main cases are where a user wants either metadata, or both
metadata and content.  This API gives just metadata; it's a
bit more efficient as we don't need to open the file, and doesn't
require the caller to `unwrap()`.
This manually adds a missing `ToGlibPtr` import, which seems to be result
of some bugs in `gir` code-generation.
Prep for merge into ostree, where we want to run `cargo fmt` checks
in CI.
I want to write APIs that *require* the caller to have set up
an ostree transaction.  It's natural to require passing a guard
to do so.  But then we want an accessor for the repo.
Fix up the paths for the crates now that the Rust bindings are in
`rust/`.

We can't today include the test suite because it depends on `ostree-rs-ext`
which would make everything circular.

(Building that now requires a separate `cd tests/inst && cargo build`)
@cgwalters
Copy link
Member Author

OK, I've reworked this to be rust-bindings/ and fixed cargo fmt incorrectly trying to reformat the sys/ bits.

As of now, this does not make ostree hard depend on Rust. New features may start to depend on Rust.

It's really tempting to remove `make syntax-check`, it has very
very rarely found any real problems.

But anyways, just exclude all the binding code because it trips
up random problems we simply don't care about like mentions of
`O_NDELAY` in the `GLib-2.0.gir`.
`tests/inst` became its own workspace.
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sane to me overall. I was expecting to see at least some CI added that builds the bindings.

rust-bindings/.github/workflows/rust.yml Outdated Show resolved Hide resolved
Need this now that it is it's own workspace.
It should replace our stub one.
We're not using Vagrant or Gitlab, and our container flow is
different.
@cgwalters
Copy link
Member Author

I was expecting to see at least some CI added that builds the bindings.

Yeah, fair. Done!

@cgwalters
Copy link
Member Author

OK, test run had a new clearly unrelated flake: coreos/fedora-coreos-tracker#1192

There's going to be a lot of followup to this, but let's get this ball rolling!

@cgwalters
Copy link
Member Author

/override continuous-integration/jenkins/pr-merge

@openshift-ci
Copy link

openshift-ci bot commented May 9, 2022

@cgwalters: Overrode contexts on behalf of cgwalters: continuous-integration/jenkins/pr-merge

In response to this:

/override continuous-integration/jenkins/pr-merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cgwalters cgwalters merged commit 891c7df into ostreedev:main May 9, 2022
cgwalters added a commit to ostreedev/ostree-rs that referenced this pull request May 9, 2022
@cgwalters
Copy link
Member Author

Followup in ostreedev/ostree-rs#55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants